-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
CI: Fixes to conda environment names #23866
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CI: Fixes to conda environment names #23866
Conversation
sorry, @datapythonista , i made a terrible mistake in Git, and collected all other people's commits ahead of me, so i created this one, and closed the previous PR so that other people won't get affected. sorry for this. |
That happens very often. Not sure what people does to get into that state, but what usually fixes it is described in the first point of this post: https://datapythonista.github.io/blog/useful-git-commands.html If you do that, you can keep using the same PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple of things that I think need to be changed. Also, if you take a look at the CI, may give more info on what is wrong
@@ -11,11 +11,10 @@ call deactivate | |||
@rem Display root environment (for debugging) | |||
conda list | |||
@rem Clean up any left-over from a previous build | |||
conda remove --all -q -y -n %CONDA_ENV% | |||
conda remove -all -q -y -n pandas-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the removed -
is needed
|
||
call activate %CONDA_ENV% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this, and another one deleted, need to be moved to .travis.yml
before those scripts are called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
oh! awesome post!!! @datapythonista thank you very much!! just read it!! i am looking into the CI now to debug what is going wrong there, and thanks for reviews!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some more comments, I think with them this should work.
This issue requires a decent knowledge of how the CI works, if too complex for you, let me know and I'll finish the PR myself.
ci/azure/linux.yml
Outdated
@@ -35,21 +35,24 @@ jobs: | |||
ci/incremental/install_miniconda.sh | |||
export PATH=$HOME/miniconda3/bin:$PATH | |||
echo "Setting up Conda environment" | |||
source activate pandas-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I'm not wrong, the environment is created in the next file, and it doesn't exist here, so it can't be activated (and it's not needed)
ci/azure/macos.yml
Outdated
@@ -22,21 +22,24 @@ jobs: | |||
ci/incremental/install_miniconda.sh | |||
export PATH=$HOME/miniconda3/bin:$PATH | |||
echo "Setting up Conda environment" | |||
source activate pandas-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as before
@rem Scipy, CFFI, jinja2 and IPython are optional dependencies, but exercised in the test suite | ||
conda env create -n %CONDA_ENV% --file=ci\deps\azure-windows-%CONDA_PY%.yaml | ||
conda env create -n pandas-dev --file=ci\deps\azure-windows-%CONDA_PY%.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need the -n pandas-dev
, the name of the environment is already defined in the yaml file
|
||
call activate %CONDA_ENV% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
|
||
echo | ||
echo "[create env]" | ||
time conda env create -q -n "${CONDA_ENV}" --file="${ENV_FILE}" || exit 1 | ||
time conda env create -q -n pandas-dev --file="${ENV_FILE}" || exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same about -n pandas-dev
not needed
ci/script_single.sh
Outdated
@@ -2,7 +2,6 @@ | |||
|
|||
echo "[script_single]" | |||
|
|||
source activate pandas | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove one of the blank lines besides de source activate and leave just one
@charlesdong1991 looks good in general, still some details that make the CI fail. Let me take a look and push couple of small fixes, and see if I can leave everything green myself. Thanks! |
@datapythonista i am so sorry that due to my limited knowledge of how CI works, i tried my best to understand what it is going on as a whole in pandas, and changed code accordingly... but it still fails... please finish the issue... sorry, i would like to help on this issue but it's bit of out of my scope... and appreciate a lot for helping me and explaining me what is going on... shall I close the PR so that you or other talented contributor can work on it? |
…1991/pandas into conda_env_name_backup_backup
Thanks for helping with this @charlesdong1991, your changes were very useful. But as you said, and as I mentioned before, a good knowledge of the CI is needed to understand in detail when the activations need to happen in some cases. Don't close the PR, all your changes are useful, and I pushed to the same PR the missing bits. With some luck now the CI is green, and we can merge the PR. Thanks again for helping with this. I'm working on simplifying the CI, and once that is done I'll add information about the CI to the docs, so it's easier for contributors to understand what's going on. |
Thanks again @datapythonista , and appreciate your patience! I tried to step out my comfort zone and solved the topic that i am not very familiar with so as to learn more... but probably this is not a good place since it will also waste your precious time to review... i will be more cautious next time when picking up issues.. thanks again, and look forward to the DOC for CI... wish this time it can go green... enjoy your weekend ^^ |
Codecov Report
@@ Coverage Diff @@
## master #23866 +/- ##
==========================================
- Coverage 92.29% 92.29% -0.01%
==========================================
Files 161 161
Lines 51497 51498 +1
==========================================
Hits 47530 47530
- Misses 3967 3968 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All green now, thanks @charlesdong1991
@jreback do you mind taking a look?
thanks @charlesdong1991 |
git diff upstream/master -u -- "*.py" | flake8 --diff